-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kafka 11 - record headers support #414
Conversation
Hey @dantswain @jbruggem @joshuawscott @bjhaid I think you are the right guys to involve here to help me (let me know if someone might be added or removed from my mentions) I am facing random errors with One can see in the CI tests each config has erros in different test cases and I am a bit lost to find a solution. Thanks for any help. |
When I released 0.11.0 I fixed most of the intermittent failures, I thought. I was able to run 20x in a row without any failures locally. That said, because Travis uses 1.5 core machines, it doesn't necessarily work the same. There does seem to be a completely consistent failure, though:
Often a single failure will cause ripples due to using the same underlying brokers, so my suggestion would be to solve this failure and see if the others begin to work. |
Hey @dantswain I would really appreciate if you could take a look into my last commit that make the ci tests pass. @joshuawscott I think now you can review and if you can help to clarify about the offsets manipulation on ℹ️ Just a remind I am pointing the |
kayrock 0.1.12 is out now |
@habutre I apologize for not getting to this yet. I have been unbelievably busy :( I will try to make time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to retry the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're good here. I kicked the build again and had one question about what seems like an unrelated change.
@@ -234,7 +287,7 @@ defmodule KafkaEx.KayrockRecordBatchTest do | |||
|
|||
fetch_responses = | |||
KafkaEx.fetch(topic, 0, | |||
offset: max(offset - 2, 0), | |||
offset: offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember exactly why this was set this way - probably some kind of bizarre test behavior. Did you have a compelling reason to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I tried to understand the reason to get a previous offset instead the one returned by the producer, but since it always worked before I kept is AS-IS. After I have introduced the headers the kayrock tests start failing randomly but always with the same reason nil
message.
I watch the topic with Kafka tool so I figure out that the offset - 2
doesn't match with the expected message then I tried to remove the max(offset - 2, 0)
and got success on every execution what make me think that was the reason and also no other test failed.
I don't have a rational about that behavior only that on my mind the offset should not be manipulated to get the desired message, it was just a recursively try-n-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dantswain in case you're interested, here is the commit where you added this max(offset - 2, 0)
: 7197a90
I didn't see anything in particular to help me understand. IMO the new changed code makes more sense 🤷
57a93c8
to
309e4c3
Compare
309e4c3
to
9b68ced
Compare
@habutre @joshuawscott |
9b68ced
to
91e9626
Compare
Hey @Argonus this PR is ready for review and I am keeping as much as possible synced with latest merged PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'd rather have a second opinion though :)
@jbruggem @joshuawscott @dantswain |
To maintain compability while using the new client the message protocol had to be changed. Only the new client is aware about such info on messages
As usual I ran the command mix format that added changes to all these files. I don't know if run the `mix format` is a desired or common practice on _KafkaEx_ contribution, so I left it in a separated commit that can be reverted without big efforts
The offset resulting from the KafkaEx.produce sometimes was decreased causing some msg not being found or retrieved correct I don't have background to know why in the compressed msg testing the offset was subtract by -2 `max(offset-2, 0)` but the tests only passed when the retrieved offset was used to fetch messages
91e9626
to
36fe923
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM. @joshuawscott @dantswain could one of you cross-check these changes to make sure it makes sense ? I'd rather not approve alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@habutre do you wish to commit those last documentation suggestions ? Once done, I'll merge. |
Co-authored-by: Jehan Bruggeman <[email protected]>
@jbruggem Maybe it's worth releasing a new version as well? |
@Argonus TBH I've never cut a release on KafkaEx before, so I'm not sure what the criteria are :). @dantswain @joshuawscott would this be a good moment to make a release, and if so who can do it ? |
This PR is targeted to #397 to provide support for record headers provided since Kafka 0.11
The headers were implemented first on
Kayrock
and for testing purposes this branch points to Kayrock master. After a new Kayrock be released this PR can be changed and merged. This change is only for your review and validation.To maintain compability while using the new client the
Protocol.{Produce, Fetch}.Message
had to be changed.Only the new client is aware about such info on messages and may not cause conflicts or misbehavior
Out of scope: The mix format causes some changes that should be evaluate if make sense or have to be reverted